-
Notifications
You must be signed in to change notification settings - Fork 13.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
KAFKA-17047: Refactored group coordinator classes to modern package (KIP-932) #16474
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is largely a mechanical change. The revised packages make sense to me.
Build passed with no new tests failure, only existing non-related tests failure. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@apoorvmittal10 nice refactor and that is more clear.
@@ -28,7 +28,7 @@ | |||
import java.util.Objects; | |||
|
|||
/** | |||
* The assignment specification for a consumer group. | |||
* The assignment specification for a modern group. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure whether this is necessary change. Maybe we should align the term: "next gen", "modern", or "CONSUMER".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your review @chia7712. I have aligned the changes to have the common functionality as modern
. As this spec class will be used in target assignment which will be also utilized by upcoming share groups
. Please let me know if there is correction needed elsewhere as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry for raising one more question
As this spec class will be used in target assignment which will be also utilized by upcoming share groups
it seems those share/consumer groups are on top of the new protocol, and the root package of those classes is called "modern". Hence, should we rename the value of group.coordinator.rebalance.protocols
from CONSUMER
to MODERN
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that we shall change the group.coordinator.rebalance.protocols
itself. There shall remain 2 for those as classic
and consumer
. Package modern
is more to share the common target assignment code between consumer
and share
groups. Share groups will be be specifically used for a consumption pattern where partitions
get shared between clients to facilitate co-operative consumption.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I raise the "naming" issue, because it is hard to distinguish the term of "consumer protocol" and "consumer instance" in introducing the "AsyncConsumer" and "CONSUMER protocol" to users. I normally give up the term used by source code and then using term "next generation" be the replacement :_
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ll defer this question to @dajac as the naming for new consumer protocol (KIP-848) defined "consumer" as the new rebalance protocol itself. But I don't see much of relation between package names and protocol name, they can be independent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@apoorvmittal10 Thanks for the patch. I left a few comments.
group-coordinator/src/main/java/org/apache/kafka/coordinator/group/GroupMetadataManager.java
Outdated
Show resolved
Hide resolved
group-coordinator/src/main/java/org/apache/kafka/coordinator/group/modern/Assignment.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks.
Build passed with unrelated tests failure. |
…KIP-932) (apache#16474) Following the discussion and suggestion by @dajac, apache#16054 (comment), the PR refactors the common classes to build TargetAssignment in `modern` package. `consumer` package has been moved inside `modern` package with classes exclusive to `consumer group`. This PR completes the refactoring and base to introduce `share` package inside `modern`. The subsequent PRs will define the implementation specific to Share Groups while re-using the common functionality from `modern` package classes. Reviewers: Andrew Schofield <[email protected]>, Chia-Ping Tsai <[email protected]>, David Jacot <[email protected]>
…KIP-932) (apache#16474) Following the discussion and suggestion by @dajac, apache#16054 (comment), the PR refactors the common classes to build TargetAssignment in `modern` package. `consumer` package has been moved inside `modern` package with classes exclusive to `consumer group`. This PR completes the refactoring and base to introduce `share` package inside `modern`. The subsequent PRs will define the implementation specific to Share Groups while re-using the common functionality from `modern` package classes. Reviewers: Andrew Schofield <[email protected]>, Chia-Ping Tsai <[email protected]>, David Jacot <[email protected]>
Following the discussion and suggestion by @dajac, #16054 (comment), the PR refactors the common classes to build TargetAssignment in
modern
package.consumer
package has been moved insidemodern
package with classes exclusive toconsumer group
.This PR completes the refactoring and base to introduce
share
package insidemodern
. The subsequent PRs will define the implementation specific to Share Groups while re-using the common functionality frommodern
package classes.Committer Checklist (excluded from commit message)